Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add self-hosted url info in Studio setup section #5089

Merged
merged 9 commits into from
Dec 13, 2023

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Dec 8, 2023

@julieg18 julieg18 added the product PR that affects product label Dec 8, 2023
@julieg18 julieg18 self-assigned this Dec 8, 2023
@@ -22,9 +27,28 @@ export const Studio: React.FC<{
)
}

const children = selfHostedStudioUrl && (
<DetailsTable>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we only see this table if the user has a self-hosted URL. But we could also adjust the table to show URL information always. This might help prem users who haven't updated as well. Example:

Screenshot 2023-12-08 at 12 08 09 PM

@julieg18 julieg18 marked this pull request as ready for review December 11, 2023 15:51
@@ -22,9 +27,28 @@ export const Studio: React.FC<{
)
}

const children = selfHostedStudioUrl && (
Copy link
Member

@mattseddon mattseddon Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] We should always show this part of the section so that users know the option is available.

When there is no URL set we can present "None" (or match what we show elsewhere on the setup page) in place of the URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I'll update the section.

})

// eslint-disable-next-line @typescript-eslint/no-explicit-any
stub(Setup.prototype as any, 'getCliCompatible').returns(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[C] Maybe this should be moved into buildSetup

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and should be stubbed on the instance (if possible)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that as well since we are running this line in multiple tests. I'll look into moving it in a followup.

text: 'Remove'
}
]
: [{ onClick: saveStudioUrl, text: 'Add Url' }]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Should this be "Add"? Does the text appear too short?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Should this be "Add"? Does the text appear too short?

Yes, the text appeared too short :)

@julieg18 julieg18 enabled auto-merge (squash) December 13, 2023 13:04
Copy link

codeclimate bot commented Dec 13, 2023

Code Climate has analyzed commit c14c1b2 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 97.7% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.2% (0.0% change).

View more on Code Climate.

@julieg18 julieg18 merged commit 4733707 into main Dec 13, 2023
7 of 8 checks passed
@julieg18 julieg18 deleted the update-setup-studio-with-prem-url branch December 13, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use correct URL for Studio Prem Environments
3 participants